-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Austenem/CAT-846 update workspace pages #3537
Conversation
Looks great! I'm working through the changes, but should we set the initial sample workspace name to the title of the example? |
I think that's a good idea! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few questions for now. I still have to check it out locally.
...ext/app/static/js/components/workspaces/NewWorkspaceDialog/NewWorkspaceDialogFromExample.tsx
Outdated
Show resolved
Hide resolved
context/app/static/js/components/workspaces/TemplateGrid/TemplateGrid.tsx
Show resolved
Hide resolved
const { isOpen: isLaunchWorkspaceDialogOpen } = useLaunchWorkspaceStore(); | ||
|
||
const { data } = useJobTypes(); | ||
const jobTypeKey = example.job_type ?? DEFAULT_JOB_TYPE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a placeholder? We don't have job_type
in the examples yet. We should set this to DEFAULT_JOB_TYPE
and leave a TODO comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I'm okay with this as long as we know the field will be called job_type
- it might save us an extra deploy in the long run
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added optional job_type
and resource_options
values to the TemplateExample
type, although neither have been added to the examples yet - both default to the default values if they are not included.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@austenem this will be a job_types
array.
...ext/app/static/js/components/workspaces/NewWorkspaceDialog/NewWorkspaceDialogFromExample.tsx
Outdated
Show resolved
Hide resolved
<Typography sx={{ mt: 2 }} variant="subtitle1"> | ||
Filter workspace templates by tags | ||
</Typography> | ||
<Stack spacing={1}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The TemplateSelectStep
is used across a number of forms. The designs no longer include this for any of them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, it does actually look like the current designs for the launch workspace dialogs still include this title - good catch!
const FlexContainer = styled('div')(({ theme }) => ({ | ||
display: 'flex', | ||
alignItems: 'center', | ||
marginBottom: theme.spacing(2), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we need the marginBottom
for all of the other instances of IconPageTitle
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't find any references to IconPageTitle
outside of workspaces, so this might be a duplicate case:
In the other cases, the SummaryData
component appears to be handling the display of this title and icon:
portal-ui/context/app/static/js/components/detailPage/summary/SummaryData/SummaryData.tsx
Lines 40 to 44 in dcc2a36
<SummaryTitle data-testid="entity-type"> | |
<SummaryDataHeader> | |
<StyledSvgIcon as={entityIconMap[entity_type]} color="primary" /> | |
{entityTypeDisplay ?? entity_type} | |
</SummaryDataHeader> |
@@ -34,7 +35,7 @@ function StepDescription({ blocks }: { blocks: (string | ReactElement)[] }) { | |||
</Stack> | |||
); | |||
} | |||
function Step({ index, title, isRequired = false, children }: PropsWithChildren<StepProps>) { | |||
function Step({ index, title, isRequired = false, hideRequiredText, children }: PropsWithChildren<StepProps>) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to set a default for hideRequiredText
to false
for the other, existing uses of Step
or does this reflect the designs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there's no default value set, it's falsy by default due to being undefined
, so it should fall back to the existing behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few places I noticed some potential duplications between our components, no major concerns 🙂
import AccordionSummary from '@mui/material/AccordionSummary'; | ||
import { styled } from '@mui/material/styles'; | ||
|
||
const StyledAccordionSummary = styled(AccordionSummary)(({ theme }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These styles seem pretty similar to the ones for the Processed Dataset accordion: https://github.com/hubmapconsortium/portal-ui/blob/25b9fea3fca9f6bc8d789b6c8851f7e44779e213/context/app/static/js/components/detailPage/ProcessedData/ProcessedDataset/styles.ts
Maybe there's some deduplication we could do? It doesn't have to be in this PR, but we should file a ticket otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh nice! Good call.
const FlexContainer = styled('div')(({ theme }) => ({ | ||
display: 'flex', | ||
alignItems: 'center', | ||
marginBottom: theme.spacing(2), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't find any references to IconPageTitle
outside of workspaces, so this might be a duplicate case:
In the other cases, the SummaryData
component appears to be handling the display of this title and icon:
portal-ui/context/app/static/js/components/detailPage/summary/SummaryData/SummaryData.tsx
Lines 40 to 44 in dcc2a36
<SummaryTitle data-testid="entity-type"> | |
<SummaryDataHeader> | |
<StyledSvgIcon as={entityIconMap[entity_type]} color="primary" /> | |
{entityTypeDisplay ?? entity_type} | |
</SummaryDataHeader> |
@@ -34,7 +35,7 @@ function StepDescription({ blocks }: { blocks: (string | ReactElement)[] }) { | |||
</Stack> | |||
); | |||
} | |||
function Step({ index, title, isRequired = false, children }: PropsWithChildren<StepProps>) { | |||
function Step({ index, title, isRequired = false, hideRequiredText, children }: PropsWithChildren<StepProps>) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there's no default value set, it's falsy by default due to being undefined
, so it should fall back to the existing behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Let's hold off on merging this until we have the examples on PROD.
Sorry to comment after already approving. Can we make sure the sort order for the templates grid is consistent across pages/forms? |
Ah good catch! All template grids should be sorted now, not just the selectable ones. |
Good to merge! |
Summary
This update adds a workspaces templates landing page, which is linked in the workspaces landing page, as well as template detail pages. These detail pages include information about the templates as well as example workspaces, which can be cloned from these pages via a new "Try Sample Workspace" dialog.
Design Documentation/Original Tickets
CAT-846 Jira ticket
CAT-819 Jira ticket (design)
Figma mockups
Testing
Template examples were tested on DEV using the one template that currently has examples (squidpy), and other examples were pulled from the most recent user-templates-api PR and hardcoded into the component for testing.
Note: attempting to clone the squidpy example workspace on DEV was unsuccessful due to the attached dataset only being available on PROD. Cloning on PROD with a hardcoded version of the example workspace was successful.
Screenshots/Video
Updated workspaces landing page:
New templates landing page:
New template detail page:
Logged in (with example):
Logged out (with example):
Logged in (no example):
New "Try Sample Workspace" dialog:
Checklist
CHANGELOG-your-feature-name-here.md
is present in the root directory, describing the change(s) in full sentences.